Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(aio): fix window title for Home page #20440

Closed
wants to merge 1 commit into from
Closed

fix(aio): fix window title for Home page #20440

wants to merge 1 commit into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Nov 15, 2017

PR Checklist

PR Type

[x] Bugfix

What is the current behavior?

Angular - link appears as the window title on the angular.io Home page.
Issue Number: #20427

What is the new behavior?

Angular appears as the window title on the angular.io Home page.
See #20427 and the commit messages for more details.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Fixes #20427.

@gkalpak gkalpak added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix labels Nov 15, 2017
@gkalpak gkalpak added this to the 5.0.x milestone Nov 15, 2017
@gkalpak gkalpak added this to REVIEW in docs-infra Nov 15, 2017
@gkalpak
Copy link
Member Author

gkalpak commented Nov 15, 2017

The first commit fixes the issue by replacing display: none with visibility: hidden on the <h1>.
The second commit proposes an alternative fix by not generating anchors for .no-anchor headings.

I prefer the second approach. Left them both for reference. I'll squash the commits before merging if we decide to go with the second one.

@mary-poppins
Copy link

You can preview 34bf92c at https://pr20440-34bf92c.ngbuilds.io/.

@gkalpak
Copy link
Member Author

gkalpak commented Nov 15, 2017

The CircleCI linting error is not related to this PR (comes from master).

petebacondarwin
petebacondarwin reviewed Nov 15, 2017
View reviewed changes
aio/tools/transforms/angular-base-package/post-processors/autolink-headings.js
properties: {className: ['material-icons']},
children: [{ type: 'text', value: 'link' }]
}
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should content be an array? Was this a mistake in the original on my part?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see that in woorm's version he wraps the content in an array.

petebacondarwin
petebacondarwin approved these changes Nov 15, 2017
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - let's go with option 2 (and squash)

aio/tools/transforms/angular-base-package/post-processors/autolink-headings.js
properties: {className: ['material-icons']},
children: [{ type: 'text', value: 'link' }]
}
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see that in woorm's version he wraps the content in an array.

@petebacondarwin petebacondarwin moved this from REVIEW to MERGE in docs-infra Nov 15, 2017
@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 15, 2017
@gkalpak
fix(aio): fix window title on Home page
8b3f6c5 
Using `display: none` on the `<h1>` causes `innerText` to not work as expected
and include the icon ligature (`link`) in the title. This caused the window
title on the angular.io Home page to appear as "Angular - link".
This commit fixes it by not generating anchors at all for headings with the
`no-anchor` class.

Fixes angular#20427
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 15, 2017
@gkalpak
Copy link
Member Author

gkalpak commented Nov 15, 2017

*squash*

@mary-poppins
Copy link

You can preview 8b3f6c5 at https://pr20440-8b3f6c5.ngbuilds.io/.

@mhevery mhevery closed this in 7e38f4f Nov 16, 2017
mhevery pushed a commit that referenced this pull request Nov 16, 2017
@gkalpak @mhevery
fix(aio): fix window title on Home page (#20440)
1285792 
Using `display: none` on the `<h1>` causes `innerText` to not work as expected
and include the icon ligature (`link`) in the title. This caused the window
title on the angular.io Home page to appear as "Angular - link".
This commit fixes it by not generating anchors at all for headings with the
`no-anchor` class.

Fixes #20427

PR Close #20440
@gkalpak gkalpak deleted the fix-aio-home-title branch November 16, 2017 11:15
@petebacondarwin petebacondarwin removed this from MERGE in docs-infra Nov 16, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
5.0.x
Development

Successfully merging this pull request may close these issues.

aio has title on the homepage set to "Angular - link" instead of just "Angular"
4 participants
@gkalpak @mary-poppins @petebacondarwin @googlebot